Added Log output for LocalQueue and ClusterQueue #3605

7h3-3mp7y-m4n · 2024-11-20T20:27:09Z

What type of PR is this?

/kind cleanup

What this PR does / why we need it:

This PR added Log output for LocalQueue and ClusterQueue

Which issue(s) this PR fixes:

Fixes #3527

Special notes for your reviewer:

It added some log output for better debugging without breaking the function..

Does this PR introduce a user-facing change?

NONE

NONE

Signed-off-by: 7h3-3mp7y-m4n <[email protected]>

k8s-ci-robot · 2024-11-20T20:27:18Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: 7h3-3mp7y-m4n
Once this PR has been reviewed and has the lgtm label, please assign mimowo for approval. For more information see the Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

k8s-ci-robot · 2024-11-20T20:27:19Z

Welcome @7h3-3mp7y-m4n!

It looks like this is your first PR to kubernetes-sigs/kueue 🎉. Please refer to our pull request process documentation to help your PR have a smooth ride to approval.

You will be prompted by a bot to use commands during the review process. Do not be afraid to follow the prompts! It is okay to experiment. Here is the bot commands documentation.

You can also check if kubernetes-sigs/kueue has its own contribution guidelines.

You may want to refer to our testing guide if you run into trouble with your tests not passing.

If you are having difficulty getting your pull request seen, please follow the recommended escalation practices. Also, for tips and tricks in the contribution process you may want to read the Kubernetes contributor cheat sheet. We want to make sure your contribution gets all the attention it needs!

Thank you, and welcome to Kubernetes. 😃

k8s-ci-robot · 2024-11-20T20:27:20Z

Hi @7h3-3mp7y-m4n. Thanks for your PR.

I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

netlify · 2024-11-20T20:27:25Z

✅ Deploy Preview for kubernetes-sigs-kueue canceled.

Name	Link
🔨 Latest commit	`bf0d9e4`
🔍 Latest deploy log	https://app.netlify.com/sites/kubernetes-sigs-kueue/deploys/674a0d887abdd300089638cc

mbobrovskyi · 2024-11-21T05:44:35Z

pkg/queue/manager.go

@@ -329,12 +330,14 @@ func (m *Manager) AddOrUpdateWorkloadWithoutLock(w *kueue.Workload) bool {
 	qKey := workload.QueueKey(w)
 	q := m.localQueues[qKey]
 	if q == nil {
+		log.Printf("LocalQueue for workload not found; queueKey: %s, workload: %s", qKey, w.Name)


I think it’s much better to return an error here with a message like 'LocalQueue for workload not found' or 'ClusterQueue for workload not found'.

for sure :)

mbobrovskyi · 2024-11-21T05:46:23Z

pkg/queue/manager.go

@@ -20,6 +20,7 @@ import (
 	"context"
 	"errors"
 	"fmt"
+	"log"


Please don't use this logger.

Okay, I'll replace it with fmt

You don't need logger at all. You need to return error instead.

k8s-ci-robot · 2024-11-21T05:47:41Z

@mbobrovskyi: /release-note-edit must be used with a release note block.

In response to this:

/release-note-edit
NONE

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

mbobrovskyi · 2024-11-21T05:47:45Z

/ok-to-test

mbobrovskyi · 2024-11-21T05:48:29Z

/release-note-edit

NONE

mimowo · 2024-11-21T08:21:45Z

/assign @gabesaba

Signed-off-by: 7h3-3mp7y-m4n <[email protected]>

mbobrovskyi · 2024-11-21T08:43:57Z

pkg/queue/manager.go

@@ -329,12 +329,14 @@ func (m *Manager) AddOrUpdateWorkloadWithoutLock(w *kueue.Workload) bool {
 	qKey := workload.QueueKey(w)
 	q := m.localQueues[qKey]
 	if q == nil {
+		fmt.Println("LocalQueue for workload not found")


Suggested change

fmt.Println("LocalQueue for workload not found")

return ErrQueueDoesNotExist

mbobrovskyi · 2024-11-21T08:44:19Z

pkg/queue/manager.go

 		return false
 	}
 	wInfo := workload.NewInfo(w, m.workloadInfoOptions...)
 	q.AddOrUpdate(wInfo)
 	cq := m.hm.ClusterQueues[q.ClusterQueue]
 	if cq == nil {
+		fmt.Println("ClusterQueue for workload not found")


Suggested change

fmt.Println("ClusterQueue for workload not found")

return ErrClusterQueueDoesNotExist

if we do this then we have to change the type of the function cause it was returning a bool and after adding this it should return error ..
so should I add it like you mentioned or anything other?

gabesaba · 2024-11-22T09:30:39Z

There are some other log lines related to this issue:

e.g. these two, as AddOrUpdateWorkload calls AddOrUpdateWorkloadWithoutLock

kueue/pkg/controller/core/workload_controller.go

Lines 602 to 604 in 2c8e7da

    
           if !r.queues.AddOrUpdateWorkload(wlCopy) { 
        
           	log.V(2).Info("LocalQueue for workload didn't exist or not active; ignored for now") 
        
           }

kueue/pkg/controller/core/workload_controller.go

Lines 744 to 746 in 2c8e7da

    
           if !r.queues.AddOrUpdateWorkload(wlCopy) { 
        
           	log.V(2).Info("LocalQueue for workload didn't exist or not active; ignored for now") 
        
           } else {

Could you track those down - there may be more than just the two I linked - and update them as well please?

7h3-3mp7y-m4n · 2024-11-22T18:15:48Z

There are some other log lines related to this issue:

e.g. these two, as AddOrUpdateWorkload calls AddOrUpdateWorkloadWithoutLock

kueue/pkg/controller/core/workload_controller.go

Lines 602 to 604 in 2c8e7da

if !r.queues.AddOrUpdateWorkload(wlCopy) {

log.V(2).Info("LocalQueue for workload didn't exist or not active; ignored for now")

}

kueue/pkg/controller/core/workload_controller.go

Lines 744 to 746 in 2c8e7da

if !r.queues.AddOrUpdateWorkload(wlCopy) {

log.V(2).Info("LocalQueue for workload didn't exist or not active; ignored for now")

} else {

Could you track those down - there may be more than just the two I linked - and update them as well please?

Yeah, I'll do that as well :)

Signed-off-by: 7h3-3mp7y-m4n <[email protected]>

7h3-3mp7y-m4n · 2024-11-26T16:25:39Z

Hey @mbobrovskyi and @gabesaba,
The PR is still a work in progress, but I wanted to share what I've done so far and check if I'm on the right track.
While working on it, I noticed a similar cache function here: workload_controller.go#L712. Should I update that as well?
Also, for the test case, since I added a new parameter in the code, I think I'll need to update the test here: manager_test.go#L421. and I think I have to add a new parameter, am I right? Thanks!

mbobrovskyi · 2024-11-26T16:23:21Z

pkg/queue/manager.go

 	}
 	wInfo := workload.NewInfo(w, m.workloadInfoOptions...)
 	q.AddOrUpdate(wInfo)
 	cq := m.hm.ClusterQueues[q.ClusterQueue]
 	if cq == nil {
-		return false
+		return errClusterQueueAlreadyExists


Suggested change

return errClusterQueueAlreadyExists

return ErrClusterQueueDoesNotExist

mbobrovskyi · 2024-11-26T16:26:05Z

pkg/controller/core/workload_controller.go

-						log.V(2).Info("LocalQueue for workload didn't exist or not active; ignored for now")
+					err := r.queues.AddOrUpdateWorkload(wlCopy)
+					if err != nil {
+						log.V(2).Info(err.Error())


Suggested change

log.V(2).Info(err.Error())

log.V(2).Info(fmt.Sprintf("%s; ignored for now", err))

mbobrovskyi · 2024-11-26T16:26:36Z

pkg/controller/core/workload_controller.go

-					log.V(2).Info("LocalQueue for workload didn't exist or not active; ignored for now")
+				err := r.queues.AddOrUpdateWorkloadWithoutLock(wlCopy)
+				if err != nil {
+					log.V(2).Info(err.Error())


Suggested change

log.V(2).Info(err.Error())

log.V(2).Info(fmt.Sprintf("%s; ignored for now", err))

mbobrovskyi · 2024-11-26T16:26:51Z

pkg/controller/core/workload_controller.go

-			log.V(2).Info("Queue for updated workload didn't exist; ignoring for now")
+		err := r.queues.UpdateWorkload(oldWl, wlCopy)
+		if err != nil {
+			log.V(2).Info(err.Error())


Suggested change

log.V(2).Info(err.Error())

log.V(2).Info(fmt.Sprintf("%s; ignored for now", err))

mbobrovskyi · 2024-11-26T16:27:18Z

pkg/controller/core/workload_controller.go

-			log.V(2).Info("LocalQueue for workload didn't exist or not active; ignored for now")
+		err := r.queues.AddOrUpdateWorkload(wlCopy)
+		if err != nil {
+			log.V(2).Info(err.Error())


Suggested change

log.V(2).Info(err.Error())

log.V(2).Info(fmt.Sprintf("%s; ignored for now", err))

To have the same message.

for sure I'll add them :)

mbobrovskyi · 2024-11-26T16:32:03Z

Hey @mbobrovskyi and @gabesaba, The PR is still a work in progress, but I wanted to share what I've done so far and check if I'm on the right track. While working on it, I noticed a similar cache function here: workload_controller.go#L712. Should I update that as well? Also, for the test case, since I added a new parameter in the code, I think I'll need to update the test here: manager_test.go#L421. and I think I have to add a new parameter, am I right? Thanks!

Yeah, we need to update tests as well.

7h3-3mp7y-m4n · 2024-11-26T16:37:25Z

Hey @mbobrovskyi and @gabesaba, The PR is still a work in progress, but I wanted to share what I've done so far and check if I'm on the right track. While working on it, I noticed a similar cache function here: workload_controller.go#L712. Should I update that as well? Also, for the test case, since I added a new parameter in the code, I think I'll need to update the test here: manager_test.go#L421. and I think I have to add a new parameter, am I right? Thanks!

Yeah, we need to update tests as well.

I'll share it with you ASAP, Thanks! @mbobrovskyi

Signed-off-by: 7h3-3mp7y-m4n <[email protected]>

mimowo · 2024-11-29T15:19:42Z

@ 7h3-3mp7y-m4n do you think you can address the comments before the 0.10 release? We are planning 4th December.

7h3-3mp7y-m4n · 2024-11-29T16:45:43Z

@ 7h3-3mp7y-m4n do you think you can address the comments before the 0.10 release? We are planning 4th December.

Hey @mimowo , I'm pretty positive that I can pull this before the release, If I jump up with some issue I'll mention it here, so no worries :)

Signed-off-by: 7h3-3mp7y-m4n <[email protected]>

7h3-3mp7y-m4n · 2024-11-29T18:50:41Z

I've temporarily suppressed the error checks for some parts. I'd appreciate it if you could guide me in fixing them as well. Thanks!

Signed-off-by: 7h3-3mp7y-m4n <[email protected]>

7h3-3mp7y-m4n · 2024-12-02T14:40:29Z

Hey @mbobrovskyi, @gabesaba can you review it ? Thanks!

Added Log output for LocalQueue and ClusterQueue

f4df8fe

Signed-off-by: 7h3-3mp7y-m4n <[email protected]>

k8s-ci-robot requested review from denkensk and mbobrovskyi November 20, 2024 20:27

k8s-ci-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Nov 20, 2024

k8s-ci-robot added the size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. label Nov 20, 2024

mbobrovskyi reviewed Nov 21, 2024

View reviewed changes

k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Nov 21, 2024

k8s-ci-robot added release-note-none Denotes a PR that doesn't merit a release note. and removed do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. labels Nov 21, 2024

k8s-ci-robot assigned gabesaba Nov 21, 2024

7h3-3mp7y-m4n added 2 commits November 21, 2024 14:09

Added changes

fa8c077

Signed-off-by: 7h3-3mp7y-m4n <[email protected]>

added println

fd7bc6a

Signed-off-by: 7h3-3mp7y-m4n <[email protected]>

mbobrovskyi reviewed Nov 21, 2024

View reviewed changes

updated the log output using default error

c77a3a1

Signed-off-by: 7h3-3mp7y-m4n <[email protected]>

k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Nov 26, 2024

mbobrovskyi reviewed Nov 26, 2024

View reviewed changes

7h3-3mp7y-m4n added 2 commits November 27, 2024 19:03

Added test case

449e24e

Signed-off-by: 7h3-3mp7y-m4n <[email protected]>

Added changes

84d415a

Signed-off-by: 7h3-3mp7y-m4n <[email protected]>

Adding return statements for ci-lint

27af647

Signed-off-by: 7h3-3mp7y-m4n <[email protected]>

k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Nov 29, 2024

removed comments

bf0d9e4

Signed-off-by: 7h3-3mp7y-m4n <[email protected]>

	fmt.Println("LocalQueue for workload not found")
	return ErrQueueDoesNotExist

	fmt.Println("ClusterQueue for workload not found")
	return ErrClusterQueueDoesNotExist

	return errClusterQueueAlreadyExists
	return ErrClusterQueueDoesNotExist

	log.V(2).Info(err.Error())
	log.V(2).Info(fmt.Sprintf("%s; ignored for now", err))

Added Log output for LocalQueue and ClusterQueue #3605

Are you sure you want to change the base?

Added Log output for LocalQueue and ClusterQueue #3605

Conversation

7h3-3mp7y-m4n commented Nov 20, 2024 • edited by k8s-ci-robot Loading

What type of PR is this?

What this PR does / why we need it:

This PR added Log output for LocalQueue and ClusterQueue

Which issue(s) this PR fixes:

Special notes for your reviewer:

Does this PR introduce a user-facing change?

k8s-ci-robot commented Nov 20, 2024

k8s-ci-robot commented Nov 20, 2024

k8s-ci-robot commented Nov 20, 2024

netlify bot commented Nov 20, 2024 • edited Loading

✅ Deploy Preview for kubernetes-sigs-kueue canceled.

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

k8s-ci-robot commented Nov 21, 2024

mbobrovskyi commented Nov 21, 2024

mbobrovskyi commented Nov 21, 2024

mimowo commented Nov 21, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gabesaba commented Nov 22, 2024

7h3-3mp7y-m4n commented Nov 22, 2024

7h3-3mp7y-m4n commented Nov 26, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mbobrovskyi commented Nov 26, 2024

7h3-3mp7y-m4n commented Nov 26, 2024

mimowo commented Nov 29, 2024

7h3-3mp7y-m4n commented Nov 29, 2024

7h3-3mp7y-m4n commented Nov 29, 2024

7h3-3mp7y-m4n commented Dec 2, 2024

7h3-3mp7y-m4n commented Nov 20, 2024 •

edited by k8s-ci-robot

Loading

netlify bot commented Nov 20, 2024 •

edited

Loading